-
Notifications
You must be signed in to change notification settings - Fork 33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactored to inject helper rather than extending it. #68
Conversation
@@ -46,6 +46,15 @@ public function __construct(SecurityContextInterface $publishWorkflowChecker = n | |||
} | |||
} | |||
|
|||
protected function getDm() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lsmith77 I think this is actually the right way to do it anyway - I believe we should also add setManagerName
and that this method should return the document manager from the registry depending on the value of $managerName ??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in this case it might however make sense to assign the manager to a local variable for those methods that use the manager multiple times.
Fixed up service.xml and added basic DI test for twig extension. |
the helper service should be tagged as a php template helper |
i am +1 on this change, looks a lot cleaner. do the tests all pass? seems travis is not looking at PR of the CoreBundle. @wouterj this is changing the refactoring you did, do you agree this makes sense? |
@dbu well, I requested to do also the change here :) It turns out that my refactoring results in an error for PHP 5.0.0 til 5.3.8, because both interfaces (template helper interface and twig extension interface) have a method So, this is the only way to do it... |
@@ -4,8 +4,15 @@ | |||
|
|||
use Symfony\Cmf\Bundle\CoreBundle\Templating\Helper\CmfHelper; | |||
|
|||
class CmfExtension extends CmfHelper implements \Twig_ExtensionInterface | |||
class CmfExtension implements \Twig_ExtensionInterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove the implement and extend Twig_Extension
and remove all method (except getName
) after line // from \Twig_Extension (line 41)
@dantleech can you do the same fixes as in the SonataBlockBundle here too? otherwise i think its ready to merge. would be good to have that in the next release. |
Updated. |
Refactored to inject helper rather than extending it.
thanks! |
thank you @dantleech for the quick fix. Shall I submit a fix for the block bundle? |
No description provided.